fix: classify upstream isError results as activity errors and gate config-load admission (#935, #937) - #944
Merged
github-actions[bot] merged 3 commits intoAug 1, 2026
Conversation
…nfig-load admission Two independent HIGH-severity core fixes. activity: an upstream answering isError:true is an error (Related #935) A tool result carrying isError:true — bad argument value, unknown tool, server-side validation failure — was recorded with status=success, because the transport hop returned err==nil and every post-dispatch emit site hardcoded "success". Only mcpproxy's own pre-dispatch validation produced status=error, so every failure consumer under-reported: the tray glance error series and its red row markers never fired for the most common real failure, and the 24h error counts were correspondingly low. activityStatusForResult() now classifies the dispatched result and supplies an error_message drawn from the result's text blocks (or its structured content), capped at 512 bytes on a rune boundary. Wired into all three dispatch paths: handleCallToolVariant, the legacy handleCallTool, and the direct-routing handler. The paired internal_tool_call record carries the same classification. Usage aggregates and the timeline derive from ActivityRecord.Status, so they become correct with no further change. What is returned to the caller is untouched — the isError result is still forwarded verbatim. The pre-dispatch error path is unchanged. security: apply the trust-mode admission gate on the config-load path (Related #937) A server written directly into mcp_config.json never passed through Config.QuarantineDefaultForServer, which was invoked only from the three add-time sites. It was admitted unquarantined with its tools auto-approved, serving poisoned tool descriptions verbatim under the default manual trust mode with quarantine_enabled:true — contradicting the trust-mode table in docs/features/security-quarantine.md. ServerConfig.UnmarshalJSON now records whether the document carried a "quarantined" key, making absent distinguishable from explicit false, and LoadConfiguredServers applies the gate before its storage-save phase. A server is gated only when the key was absent AND the server is not yet in config.db. Every server an existing user runs already has a config.db record, so upgrading re-quarantines nothing. The gate only ever adds quarantine, never removes it.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30688558068 --repo smart-mcp-proxy/mcpproxy-go
|
Cross-model review of PR #944 found six ways the gate could be bypassed, inverted, or raced. All six are fixed with a failing test first. P1 — any mcpproxy-initiated config save permanently disarmed the gate. ServerConfig.Quarantined had no omitempty and SaveConfig is a plain MarshalIndent, so one write of the config file stamped "quarantined": false onto every server; reading that back set the presence bit and the gate skipped admission forever — including for servers it had just quarantined, whose config.db record the next start then overwrote with false. ServerConfig now has a MarshalJSON that omits the key when the value is false and no document ever stated it, and always writes it when true. P2 — a transient storage read failure mass-quarantined everything. A failed ListUpstreamServers() was flattened into "no servers are known", so every configured server looked first-seen and was walled off, permanently. The gate now takes an explicit storage-readable flag and abstains when it is false. P2 — the gate mutated an already-published config snapshot. It wrote through the *config.ServerConfig pointers the supervisor and serverEligibleForIndexing read concurrently, and both hot paths published before gating. The gate now returns a copy and never writes through its input; configsvc gained a pre-publish hook so every configuration is gated before any subscriber can observe it; ApplyConfig gates before its disk write (covering the RequiresRestart branch, which never reaches LoadConfiguredServers); and the startup snapshot is gated before supervisor.Start(). P2 — installs already hit by #937 stay live after upgrading, because the buggy admission left exactly the config.db record the gate reads as vetted. That tradeoff stays (upgrade safety), but it is no longer silent: startup names the affected servers in a warning, and the docs carry an "action required" upgrade section. A human quarantine toggle now records itself as an explicit value in the config document, so a reviewed server is never reported. P2 — "quarantined": null counted as an operator statement, contradicting the codebase's own RFC 7396 null-means-unset convention and letting a templating tool admit a first-seen server. Null is now absence. P2 — code_execution is a fourth upstream dispatch path and still classified on err == nil alone, so an upstream CallToolResult{IsError:true} was stored in tool-call history as a clean success while the identical call through call_tool_read recorded the upstream's message. Nested calls now share the issue #935 classifier; the code_execution wrapper deliberately keeps its own result.Ok outcome, documented inline. Related #937 Related #935
Deploying mcpproxy-docs with
|
| Latest commit: |
c64f128
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82e575b6.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-935-937-activity-error-a.mcpproxy-docs.pages.dev |
Member
Author
Cross-model review fixes (round 2)All six confirmed findings are fixed, each with a failing test written first.
VerificationDeliberately not done
|
…sponse config.ServerConfig gained custom MarshalJSON/UnmarshalJSON in the #937 config-load admission gate work. Go promotes those methods to any struct that embeds it, and api.ServerResponse embeds *config.ServerConfig, so encoding/json treated the whole wrapper as a Marshaler/Unmarshaler and delegated to the embedded config: - decode failed with "json: Unmarshal(nil *config.Alias)" (the embedded pointer is nil before decoding starts), turning every test in internal/serveredition/api red; - encode silently DROPPED "ownership" and "user_enabled" from every server-edition API response, and panicked outright on a nil embedded config. No test covered that, so it would have shipped as a silent API regression. Give ServerResponse explicit JSON methods that marshal the embedded config through its own MarshalJSON — preserving the #937 quarantined presence semantics exactly — and splice the wrapper fields on top. ServerResponse is the only type in the repo that embeds ServerConfig; document the promotion hazard on the config methods so the next one does not repeat it. Related #937
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Live QA verified: upstream isError:true now recorded as status=error across all three dispatch paths (with usage aggregates/timeline agreeing), and config-load servers pass the trust-mode admission gate — upgrade-safe (only servers with no quarantined key AND absent from config.db). Post-review fix: explicit ServerResponse JSON methods, after proving the promoted MarshalJSON was silently dropping ownership/user_enabled from every server-edition response.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent HIGH-severity core fixes, one branch.
#935 — an upstream returning
isError: truewas recorded asstatus=successWhat was wrong. A tool result carrying
isError: true(bad argument value, unknown tool, server-side validation failure) is a failed tool call that arrives over a successful transport hop:err == nil. Every post-dispatch emit site hardcoded"success", so only mcpproxy's own pre-dispatch validation ever producedstatus=error. Consequences: the tray glance error series and its red ⊗ row markers never fired for the most common real failure, the 24h error counts were low, and a human debugging "why did my tool call not work" saw a green success row.What changed.
activityStatusForResult()(internal/server/activity_result_status.go) classifies a dispatched result and extracts anerror_messagefrom its text blocks, falling back to its structured content and then to a fixed string. The message is capped at 512 bytes on a rune boundary (activity records are persisted and streamed to the UI).handleCallToolVariant, the legacyhandleCallTool, and the direct-routing handler inmcp_routing.go.internal_tool_callrecord carries the same classification (per (c) in the brief) — matching what the pre-existingerr != nilbranch already did.ToolCallRecord.Erroris mirrored so the tool-call history agrees with the activity log.Consumers that become correct for free (per (d)):
runtime.UsageAggregate.Applyderives per-toolErrorsand the timeline'sErrorsbucket straight fromActivityRecord.Status, so the aggregates and the 24h histogram are fixed by the same change. No aggregate code was touched.Explicitly preserved (per (a) and (b)): the caller still receives the upstream's
isErrorresult verbatim — only the activity classification changed; the pre-dispatch error path is untouched.Deliberately deferred. The issue asks whether an upstream-reported error should be a distinct status from a proxy-side error. It should, but not here: every consumer (usage aggregate, timeline, tray, REST filters, frontend) switches on the two known values, and a third value would be silently dropped by all of them. That migration needs to move the consumers together. The
error_messagealready carries the upstream's own words, which is what a human debugging the call needs. Rationale is recorded in the file's doc comment.Known, pre-existing.
ActivityFilter.ExcludeCallToolSuccessonly drops successfulcall_tool_*internal records, so a failed dispatch shows both atool_callrow and aninternal_tool_callrow. That was already true for transport failures;isErrorcalls now join them. Changing the dedup is a UI-behaviour change and was left out of scope.#937 — servers loaded from
mcp_config.jsonbypassed the trust-mode admission gateWhat was wrong.
Config.QuarantineDefaultForServer()was invoked from exactly three add-time sites. A server written straight intomcp_config.jsonreached the upstream manager without passing any of them: admitted unquarantined, tools auto-approved, poisoned description served verbatim — under the defaultmanualtrust mode withquarantine_enabled: true.ServerConfig.Quarantinedbeing a plainboolmeant an absentquarantinedkey was indistinguishable from an explicitfalse.What changed.
ServerConfig.UnmarshalJSONnow records whether the parsed document actually carried aquarantinedkey (unexported bit, never serialized — it describes the document, not the server). Exposed asQuarantineExplicitlySet(). Carried throughCopyServerConfig.Runtime.applyConfigLoadAdmissionGateruns insideLoadConfiguredServers, before the storage-save phase, so the gated value is what gets persisted, connected and reported.Gating rule — a server is held only when both hold:
quarantinedvalue (writing"quarantined": falseis an operator statement and is obeyed), andconfig.db.Upgrade safety. Condition 2 is the "has already been through admission" signal. Every server an existing user is running already has a
config.dbrecord, so an upgrade re-quarantines nothing.The boundary, stated precisely (also in the code comment and in
docs/features/security-quarantine.md): a server present in a hand-written config but absent fromconfig.dbis treated as first-seen and gated. That happens after a wiped data directory, or when a config file lands on a machine whose data dir has never seen it — which is exactly the "config files get shared, templated, copied between machines" case the issue calls out, so gating it is intended, not collateral.Durability. Once gated, the decision is persisted. On the next start the server is in
config.dbwhile the file still says nothing, so the un-stated key inherits mcpproxy's own recorded state instead of resolving back tofalse— without that, the hole would reopen one restart later.Direction. Both branches are guarded on
!sc.Quarantined: the gate can only ever add quarantine, never remove it. A stale storage record can never relax a config (or an in-flight add-path struct) that already asks for quarantine. Un-quarantining stays a user action throughRuntime.QuarantineServer, which writes both stores.Not done. No TPA scan at config-load admission — quarantine-by-default already keeps the tools away from agents, and a synchronous scan would block boot on every first-seen server. Noted in the code comment and the docs.
Not changed:
Quarantinedstays aboolrather than becoming*bool(the issue's suggested direction). 234 call sites across the repo read it; the presence bit gets the same discrimination with no blast radius.Verification
#935 — red first
The AST guard, after the classifier existed but before the call sites were wired:
And the end-to-end test — a real mock upstream returning
isError: true, driven through client →/mcp→call_tool_read→ upstream, asserting on the persisted activity record:#935 — green
#937 — red first
Exactly the two security-relevant cases were red; the upgrade-safety and opt-out cases passed from the start and stand as regression guards.
#937 — green
MatchesAddPathDecisionis the issue's own comparison table as an assertion: for every trust mode, the config-load verdict must equal whatConfig.QuarantineDefaultForServergives the add path — which is also the proof that add-path behaviour is unchanged (it is the reference the gate is measured against, and no add-path code was touched).TestSaveServerSyncFieldCoverageininternal/storagecaught the new struct field and was updated with a comment explaining why the parse-time bit is deliberately not persisted.Full gates
Also run clean as blast-radius checks on the new
ServerConfig.UnmarshalJSON:./internal/httpapi/...,./internal/contracts/...,./cmd/....No REST annotations changed, so no
make swaggerneeded; the pre-push hook's "Verify OpenAPI spec is up to date" passed.Related #935
Related #937